Code review for statisticians, data scientists, & modellers

Dr Jack Kennedy (he/him)

Infectious disease modelling team | UKHSA

I’m Jack, it’s nice to meet you 👋

statistics PhD, education, & consultancy

data scientist & infectious disease modeller

bit of git nerd

I mostly work in R, concepts are language-agnostic

Who is this talk for?

🎓 a big chunk of my team have an academic background

👩‍💻 we spend a lot of time developing software …

📊 … but we’re in analytical roles

🤫 honestly? This talk is for me …

It’s a balancing act

Have you ever …

👩‍💻 submitted a pull request?

🔍 performed a code review?

🤔 thought about how to do code review better?

vote with 👍 and 👎 emojis

Literal code review

📝 write some comments

💻 run the code

🔧 make direct suggestions

👀 let’s review some code

Why review code?

✔️ quality & compliance

💡 provide ideas for improving the code

🧠 knowledge transfer

🚌 increase your bus number!

What is a reviewer looking for?

🐛 potential bugs

📈 quality of outputs

🔢 check the mathematics and statistics

💼 sensitive information!

How should a reviewer behave?

❤️ be kind and praise your team members

☕ take a break if you’re getting grumpy

❓ ask open ended questions about the code

🤖 automate the painful stuff: linters, stylers, CI/CD

really important to approach review with a kind, open minded perspective. We all want to deliver quality products and we’re all working really hard to do that

reviewing long or highly technical pull requests can be a difficult experience which leads to the reviewing being blunt or grumpy. This is natural human behaviour, but this is something we should aim to counteract. Take a break if you’re reviewing for more than say, an hour. Take your own patient into account. Before you submit the review, it can be worth leaving it and re-reading all your comments/suggestions

asking open ended questions about the code, rather than making opinionated statements is really helpful. For example “can you explain why you’ve taken this approach?” is a much nicer line of questioning than “this is bad, change to X”. It’s a good idea to adopt an unassuming tone

we can avoid a lot of grumpiness by automating the tedious parts code review. Use a style guide and stick to it (rapid response is a reasonable reason not to be overly critical). Use linters and stylers to help team members conform to a style guide. CI/Cd tools and precommit hooks can prevent the reviewer with getting lumped with code that doesn’t work or fails to meet stylistic requirements

Code is never perfect

🩺 accept PRs when overall health is improved

☎️ have a natter about the code

🛠️ favour maintainable and readable code

👪 broaden the discussion

sometimes a reviewer and author can get into a really long game of github ping pong and can’t settle on what the “right” code solution to a problem is.

I remember when I was in the first year of my PhD, a lecturer in my department said to me “a thesis is never finished, only submitted” - software projects are like this too. There’s always more that can be done.

so how can we ensure we’re getting code moving, rather than an infinite game of github ping pong?

we should aim to accept PRs when the overall health of code and analytical outputs is improved. If it’s not quite perfect. A good way to keep code moving is keep the scope of tickets/PRs narrow and make sure you’ve written up issues to resolve in the future. For example, ticket 1 might be to write the infrastructure for a modelling pipeline, ticket 2 might be to improve the model in that pipeline. Doing both in one step is going to be challenging.

when we’re working asynchronously, a lot can get missed in written communication. For complex PRs/code, having a conversation about the code can be really useful

when I’m in a review stalemate, I tend to favour code which is maintainable and readable over performant or clever. I recently read something to the effect of “debugging is twice as hard as writing” so if the code is at our intellectual limit, you’re going to have no fun debugging. We’re probably going to spend longer maintaining our code than running.

it can also be a good idea to just ask others in your team when you’re stuck. An additional opinion can break a stalemate and provide additional experience

Triage your suggestions

🕷️ Nit: this is minor; I am being pedantic

🤔 Consider: I’m not too bothered, would be nice

🤷 FYI: out of scope, I think you’d like this approach

⚠️ Should: an essential fix!

I find for really long reviews, if can be overwhelming for the reviewing if you’ve left a lot of comments - completely understandable. Using the given scheme can reduce the stress a little

a good tactic here is to let the author know how important everything is. Nit picks aren’t a hill to die on, and automating aspects of code review with e.g. linters is going to reduce them massively

i also find that sometimes people don’t want to make changes because they think it’s going to take a long time to resolve the problem. There’s a few considerations here

decide how important the fix is
ask them to try anyway, if it takes too long (say, more than 20 mins), consider spinning up a ticket to resolve the issue
PRs don’t have to merge into main, so you can always have someone else work on the fix and merge the branches together before pushing to main

Every team is unique

📰 come up with your own team code review style

📣 agree on the style as a team activity

♻️ trial it, review it, adapt it

🖊️ document your process (use a wiki!)

Some thoughts on best practices

best practices are always changing

🤔 come up with a sensible approach

🖊️ document it, shout about it, adapt it

🎯 apply the rules consistently

🤷 anything else is just an opinion

why adopt best practices?

⛵ we run a tight ship

😩 decision fatigue

💎 clarity leads to productivity

🤷 maybe they should be called standard practices

In review

🔑 code review is key to quality outputs

👪 we all need to be reviewing code

👏 be kind, supportive and encouraging

⏩ keep code moving forward

Further resources

Keep in touch 🖊️

@_jcken@fossoton.org

Connect with me!

Chat on Gov DS

Maybe see you at a north-east tech meetup?

Questions, comments, thoughts?

Bonus content

well done for getting this far …